-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLIENT-SPECIFICATION: update asset URLs to use GitHub releases, disambiguate triple-brace placeholders #12158
Conversation
…biguate triple-brace placeholders
Merge mine and then rebase? Why a second PR? |
Feel free to do whichever method you prefer, there seems to be some additional changes here (if you can incorporate it into your PR that works for me too). |
We're not releasing the new spec yet, are we? Was there some discussion I missed? I don't think this was supposed to be merged. |
Yeah, we haven't discussed about merging this yet or how to deal with sbrl's PR. Since it got merged, no need to revert it, I will check what to do later today (if any changes are required will open a new PR or modify Sbrl's). We aren't going to make a new release yet, as we are discussing something about funding (moving to Open Collective, etc) in the chatroom thread, feel free to join the conversation over there. |
…biguate triple-brace placeholders (tldr-pages#12158)
AFAIK funding discussion on Matrix (which should really be repeated in an issue I think) stalled due to UK advertisement law that could hold some maintainers personally liable. That discussion probably shouldn't block this one or a release, since discussion of any funding etc should probably be discussed in a dedicated release post / etc? |
Agreed, then IG we can discuss it in a later date (ideally on an issue). Can you check the client specification and see if the changed wording is fine? I will try to make a release this weekend if everything is good (IG we can go with version 2.5 for now and when the actual change is done [removal of assets from website] we can make a major release 3.0). |
Sounds good.
Sure thing! As ofhttps://github.com/tldr-pages/tldr/blob/60640ac3519aa9901177d5bfe7decbbcec8f44cf/CLIENT-SPECIFICATION.md#caching, which is the latest change to
Sounds good! Could you make sure to mention in the release at least that the existing URLs for download are NOT going away to assuage any fears from client spec implementors? Might be worth a quick update to the spec changelog too ref this. Many thanks for all your hard work! |
Mention that it isn't going away now, but in the future one? We discussed about continuing the website method but it would still have the storage issue and would introduce additional maintenance. Maybe, we could add a redirect for the main tldr.zip asset alone? [For backward compatibility with clients not updated yet] (To redirect from the website to the GitHub assets link; I haven't tested this one yet so not sure how it would work as it can't be done in the DNS side from Cloudflare so we need to setup a normal HTML redirect using GitHub pages under the assets directory.) |
A redirect would work if that works with GitHub pages? Note it would need to generate the appropriate HTTP status code 'cause clients won't parse HTML. Or else just stop updating the zip on the website? |
Unfortunately, making 301 redirects with GitHub pages is impossible. The only way to do it is via a redirect $ curl https://tldr.sh/chatroom -Lsw "\nResponse code: %{response_code}\n"
<!DOCTYPE html>
<html lang="en">
<head>
<title>TLDR Pages Chatroom</title>
<meta http-equiv="refresh" content="0; url=https://matrix.to/#/#tldr-pages:matrix.org" />
</head>
</html>
Response code: 200 I think the best way to tackle this is to put a notice in the release notes saying that the old URL is now deprecated, and then stop publishing assets after some time. After that we can proceed with the changes to the website repo proposed in #12048 (comment). |
Agreed, I will try to test the meta tag redirect to the GitHub release asset of complete zip alone (since a majority of clients still use it), but I don't think we should advertise this method in release notes and instead keep it just for compatibility with a lot of abandoned clients. Edit. I initially thought of making a release today, but let's discuss this thoroughly first. So I will postpone the client specification release a few more days IG. |
As @sbrl said, clients would have to parse HTML for this to work. Just try to send a GET request to https://tldr.sh/chatroom with any HTTP client that isn't a web browser - it will return this instead of redirecting to Matrix. |
#11949 (comment) & #12062